[clang][bytecode] Always initialize scope before conditional operator #171133
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise, the scope might be (lazily) initialized in one of the arms of the conditional operator, which means the will NOT be intialized in the other arm. Fixes llvm#170981
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) ChangesOtherwise, the scope might be (lazily) initialized in one of the arms of the conditional operator, which means the will NOT be intialized in the other arm. Fixes #170981 Full diff: https://github.com/llvm/llvm-project/pull/171133.diff 3 Files Affected:
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 14b6bb7bf61d5..ed5493c315dd7 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -2503,6 +2503,15 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
const Expr *TrueExpr = E->getTrueExpr();
const Expr *FalseExpr = E->getFalseExpr();
+ if (std::optional<bool> BoolValue = getBoolValue(Condition)) {
+ if (*BoolValue)
+ return this->delegate(TrueExpr);
+ return this->delegate(FalseExpr);
+ }
+
+ // Force-init the scope, which creates a InitScope op. This is necessary so
+ // the scope is not only initialized in one arm of the conditional operator.
+ this->VarScope->forceInit();
// The TrueExpr and FalseExpr of a conditional operator do _not_ create a
// scope, which means the local variables created within them unconditionally
// always exist. However, we need to later differentiate which branch was
@@ -2510,13 +2519,6 @@ bool Compiler<Emitter>::VisitAbstractConditionalOperator(
// "enabled" flags on local variables are used for.
llvm::SaveAndRestore LAAA(this->VarScope->LocalsAlwaysEnabled,
/*NewValue=*/false);
-
- if (std::optional<bool> BoolValue = getBoolValue(Condition)) {
- if (*BoolValue)
- return this->delegate(TrueExpr);
- return this->delegate(FalseExpr);
- }
-
bool IsBcpCall = false;
if (const auto *CE = dyn_cast<CallExpr>(Condition->IgnoreParenCasts());
CE && CE->getBuiltinCallee() == Builtin::BI__builtin_constant_p) {
diff --git a/clang/lib/AST/ByteCode/Compiler.h b/clang/lib/AST/ByteCode/Compiler.h
index c641af52c2811..1bd15c3d79563 100644
--- a/clang/lib/AST/ByteCode/Compiler.h
+++ b/clang/lib/AST/ByteCode/Compiler.h
@@ -505,6 +505,7 @@ template <class Emitter> class VariableScope {
virtual bool emitDestructors(const Expr *E = nullptr) { return true; }
virtual bool destroyLocals(const Expr *E = nullptr) { return true; }
+ virtual void forceInit() {}
VariableScope *getParent() const { return Parent; }
ScopeKind getKind() const { return Kind; }
@@ -534,7 +535,6 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
this->Ctx->emitDestroy(*Idx, SourceInfo{});
removeStoredOpaqueValues();
}
-
/// Explicit destruction of local variables.
bool destroyLocals(const Expr *E = nullptr) override {
if (!Idx)
@@ -558,10 +558,22 @@ template <class Emitter> class LocalScope : public VariableScope<Emitter> {
this->Ctx->Descriptors[*Idx].emplace_back(Local);
}
+ /// Force-initialize this scope. Usually, scopes are lazily initialized when
+ /// the first local variable is created, but in scenarios with conditonal
+ /// operators, we need to ensure scope is initialized just in case one of the
+ /// arms will create a local and the other won't. In such a case, the
+ /// InitScope() op would be part of the arm that created the local.
+ void forceInit() override {
+ if (!Idx) {
+ Idx = static_cast<unsigned>(this->Ctx->Descriptors.size());
+ this->Ctx->Descriptors.emplace_back();
+ this->Ctx->emitInitScope(*Idx, {});
+ }
+ }
+
bool emitDestructors(const Expr *E = nullptr) override {
if (!Idx)
return true;
- assert(!this->Ctx->Descriptors[*Idx].empty());
// Emit destructor calls for local variables of record
// type with a destructor.
diff --git a/clang/test/AST/ByteCode/cxx20.cpp b/clang/test/AST/ByteCode/cxx20.cpp
index ea4843e95b01f..227f34cee80ff 100644
--- a/clang/test/AST/ByteCode/cxx20.cpp
+++ b/clang/test/AST/ByteCode/cxx20.cpp
@@ -1211,3 +1211,17 @@ namespace DyamicCast {
constexpr const X *p = &y;
constexpr const Y *q = dynamic_cast<const Y*>(p);
}
+
+namespace ConditionalTemporaries {
+ class F {
+ public:
+ constexpr F(int a ) {this->a = a;}
+ constexpr ~F() {}
+ int a;
+ };
+ constexpr int foo(bool b) {
+ return b ? F{12}.a : F{13}.a;
+ }
+ static_assert(foo(false)== 13);
+ static_assert(foo(true)== 12);
+}
|
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/28604 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Otherwise, the scope might be (lazily) initialized in one of the arms of the conditional operator, which means the will NOT be intialized in the other arm.
Fixes #170981